Skip to content

refactor: use Optional return types instead of nullable boxed primitives in public API#189

Open
Copilot wants to merge 8 commits into
mainfrom
copilot/update-public-api-to-use-optional
Open

refactor: use Optional return types instead of nullable boxed primitives in public API#189
Copilot wants to merge 8 commits into
mainfrom
copilot/update-public-api-to-use-optional

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented May 11, 2026

Resolves #187


Before the change?

  • ~30 public getters on mutable config/builder classes return nullable boxed types (Boolean, Integer, Double) — a C# idiom carried over from the reference implementation. The "may be absent" contract is invisible in type signatures and forgotten null-checks compile fine but NPE at runtime.

After the change?

  • Getters return Optional<Boolean>, OptionalInt, or OptionalDouble. Setters take primitives (boolean, int, double). New clearXxx() methods reset to null (server default).
  • Internal fields remain nullable boxed types — Jackson serialization is unchanged. @JsonIgnore on Optional-returning getters prevents Jackson from attempting to serialize Optional wrappers.
  • All callers updated from null-check patterns to Optional APIs:
// Before
request.setEnableSessionTelemetry(config.getEnableSessionTelemetry());

// After
config.getEnableSessionTelemetry().ifPresent(request::setEnableSessionTelemetry);

Affected classes (15):

  • Public config: CopilotClientOptions, SessionConfig, ResumeSessionConfig, InfiniteSessionConfig, InputOptions, ModelCapabilitiesOverride.Supports, ModelCapabilitiesOverride.Limits, ProviderConfig, TelemetryConfig, SessionUiCapabilities, CustomAgentConfig, UserInputRequest
  • Wire DTOs: CreateSessionRequest, ResumeSessionRequest, McpServerConfig, ModelLimits

Updated callers: CliServerManager, CopilotClient, CopilotSession, SessionRequestBuilder

Not changed: Records, generated code under src/generated/java/, primitive fields that were already non-nullable.

Pull request checklist

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • mvn spotless:apply has been run to format the code
  • mvn clean verify passes locally

Does this introduce a breaking change?

  • Yes
  • No

Getter return types change from Boolean/Integer/DoubleOptional<Boolean>/OptionalInt/OptionalDouble. Setter parameter types change from boxed → primitive. Callers passing null to setters must use the new clearXxx() methods instead.


Copilot AI and others added 2 commits May 12, 2026 00:33
…ves in public API

Change getter return types from nullable Boolean/Integer/Double to
Optional<Boolean>/OptionalInt/OptionalDouble on all mutable config/builder
classes. Setters now take primitive parameters. Add clearXxx() methods
for resetting to null (server default). Add @JsonIgnore on Optional-returning
getters to preserve Jackson serialization. Update all callers and tests.

Co-authored-by: edburns <75821+edburns@users.noreply.github.com>
Co-authored-by: edburns <75821+edburns@users.noreply.github.com>
Copilot AI changed the title [WIP] Update public API to use Optional return types refactor: use Optional return types instead of nullable boxed primitives in public API May 12, 2026
Copilot AI requested a review from edburns May 12, 2026 00:45
@edburns
Copy link
Copy Markdown
Collaborator

edburns commented May 13, 2026

@copilot

Consider the attached file pr-189-report-card.md as a report card on your work.

This text cites the most important things to fix in your next iteration of the work. You can dispute the grade if you have a good reason that you did it the way you did. If not, please just heed the comments. The items in this list reference sections in the report card.

  1. "1.5 InputOptions.javaB"

    • I don't see why this one is missing @JsonIgnore. If there is no good reason not to add it, please add it.
  2. "Internal Wire DTOs"

    • McpServerConfig.java

    • ModelLimits.java

    Roll back the changes to these classes. You did a great job by not doing the change on the CreateSessionRequest and ResumeSessionRequest classes, so I now judge it is better to not do any of these changes to the "Internal Wire DTO" classes.

  3. "Section 4: Testing"

Heed all the comments in this section. ✅✅This is most important✅✅.

pr-189-report-card.md

Copilot AI and others added 3 commits May 13, 2026 00:13
…imits changes

Co-authored-by: edburns <75821+edburns@users.noreply.github.com>
…p tests

Co-authored-by: edburns <75821+edburns@users.noreply.github.com>
Co-authored-by: edburns <75821+edburns@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented May 13, 2026

@copilot

Consider the attached file pr-189-report-card.md as a report card on your work.
...

All three items addressed:

  1. InputOptions.java — Added @JsonIgnore to both getMinLength() and getMaxLength() getters, matching the pattern used in all other classes.

  2. Internal Wire DTOs — Rolled back all changes to McpServerConfig.java, ModelLimits.java, McpHttpServerConfig.java, and McpStdioServerConfig.java. Also reverted the corresponding test changes in DataObjectCoverageTest and MetadataApiTest.

  3. Testing — Created OptionalApiAndJacksonTest.java with:

    • 24 individual clearXxx() tests covering all public API config classes (CopilotClientOptions, SessionConfig, ResumeSessionConfig, InfiniteSessionConfig, InputOptions, ModelCapabilitiesOverride.Supports, ModelCapabilitiesOverride.Limits, ProviderConfig, TelemetryConfig, SessionUiCapabilities, CustomAgentConfig, UserInputRequest)
    • 3 Jackson serialization roundtrip tests verifying that clearXxx() causes fields to be omitted from JSON output (for classes with @JsonProperty-annotated fields: InfiniteSessionConfig, ModelCapabilitiesOverride.Supports, ModelCapabilitiesOverride.Limits)

@edburns edburns marked this pull request as ready for review May 13, 2026 01:42
Copilot AI review requested due to automatic review settings May 13, 2026 01:42
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors the SDK’s mutable config/builder surface area to make “optional/absent” values explicit in Java type signatures by changing nullable boxed-primitive getters to Optional<Boolean>, OptionalInt, and OptionalDouble, adding clearXxx() methods, and updating request-building/call sites accordingly while preserving Jackson wire behavior.

Changes:

  • Replace nullable boxed primitive getters with Optional* getters and change setters to primitives, adding clearXxx() methods across config/DTO classes.
  • Update request-building and runtime call sites to use Optional APIs (ifPresent, orElse) instead of null checks.
  • Update and add tests to validate Optional-based contracts and ensure Jackson omits cleared (null) fields.
Show a summary per file
File Description
src/main/java/com/github/copilot/sdk/json/CopilotClientOptions.java Converts optional boxed getters to Optional/OptionalInt, adds clear methods, and updates docs for the new API shape.
src/main/java/com/github/copilot/sdk/json/SessionConfig.java Updates optional boolean config getters to Optional<Boolean>, setters to primitives, and adds clearXxx() methods used by request building.
src/main/java/com/github/copilot/sdk/json/ResumeSessionConfig.java Mirrors SessionConfig Optional/clear pattern for resume configuration.
src/main/java/com/github/copilot/sdk/json/InfiniteSessionConfig.java Switches tri-state fields to Optional getters + primitive setters + clear methods while retaining Jackson field serialization.
src/main/java/com/github/copilot/sdk/json/InputOptions.java Updates min/max length to OptionalInt getters and primitive setters + clear methods (used in UI input flow).
src/main/java/com/github/copilot/sdk/json/ProviderConfig.java Updates token limit overrides to OptionalInt getters, primitive setters, and clear methods.
src/main/java/com/github/copilot/sdk/json/TelemetryConfig.java Updates capture-content flag to Optional getter, primitive setter, and clear method (consumed when starting CLI).
src/main/java/com/github/copilot/sdk/json/SessionUiCapabilities.java Updates elicitation flag getter to Optional and ensures Jackson mapping via field annotation.
src/main/java/com/github/copilot/sdk/json/CustomAgentConfig.java Updates infer flag to Optional getter, primitive setter, and clear method.
src/main/java/com/github/copilot/sdk/json/UserInputRequest.java Updates allowFreeform to Optional getter, primitive setter, and clear method while keeping JSON property mapping.
src/main/java/com/github/copilot/sdk/json/CreateSessionRequest.java Updates request DTO setters for optional booleans to primitives and adds clear methods.
src/main/java/com/github/copilot/sdk/json/ResumeSessionRequest.java Updates request DTO setters for optional booleans to primitives and adds clear methods.
src/main/java/com/github/copilot/sdk/SessionRequestBuilder.java Updates request construction to use Optional APIs and avoid writing null/false unless explicitly configured.
src/main/java/com/github/copilot/sdk/CliServerManager.java Updates CLI argument/env var derivation to use Optional getters and preserve defaulting semantics.
src/main/java/com/github/copilot/sdk/CopilotClient.java Updates validation logic to treat “auth option explicitly set” via Optional presence.
src/main/java/com/github/copilot/sdk/CopilotSession.java Updates elicitation capability checks and model capability mapping to work with Optional-returning getters.
src/test/java/com/github/copilot/sdk/CopilotClientTest.java Updates tests to assert Optional return values for client options.
src/test/java/com/github/copilot/sdk/ConfigCloneTest.java Updates clone-related tests to use Optional semantics and clear methods.
src/test/java/com/github/copilot/sdk/DataObjectCoverageTest.java Updates coverage assertions to use Optional getters.
src/test/java/com/github/copilot/sdk/ElicitationTest.java Updates elicitation-related assertions and request-building expectations for new APIs.
src/test/java/com/github/copilot/sdk/ProviderConfigTest.java Updates serialization/roundtrip assertions to use OptionalInt.
src/test/java/com/github/copilot/sdk/TelemetryConfigTest.java Updates telemetry flag tests to use Optional<Boolean>.
src/test/java/com/github/copilot/sdk/OptionalApiAndJacksonTest.java Adds targeted tests for clear methods, Optional getter behavior, and Jackson omission of cleared fields.

Copilot's findings

Comments suppressed due to low confidence (2)

src/test/java/com/github/copilot/sdk/ElicitationTest.java:190

  • Same concern as above: unboxing a nullable Boolean in an assertion can produce an NPE instead of an assertion failure. Prefer assertEquals(Boolean.TRUE, request.getRequestElicitation()) (or Boolean.TRUE.equals(...)).
        var request = SessionRequestBuilder.buildResumeRequest("session-1", config);

        assertTrue(request.getRequestElicitation());
    }

src/main/java/com/github/copilot/sdk/json/ModelCapabilitiesOverride.java:218

  • The Limits Javadocs still describe returning/accepting null to use the runtime default, but the getters now return OptionalInt and setters take primitives with clearXxx() for reverting to default. Please update the @return/@param docs to reflect OptionalInt.empty() and the clear methods rather than null.
    /**
     * Token limit overrides for model capabilities.
     * <p>
     * Set a field to override that limit; leave it {@code null} to use the runtime
     * default.
     */
    @JsonInclude(JsonInclude.Include.NON_NULL)
    @JsonIgnoreProperties(ignoreUnknown = true)
    public static class Limits {

        @JsonProperty("max_prompt_tokens")
        private Integer maxPromptTokens;

        @JsonProperty("max_output_tokens")
        private Integer maxOutputTokens;

        @JsonProperty("max_context_window_tokens")
        private Integer maxContextWindowTokens;

        /**
         * Gets the maximum prompt tokens override.
         *
         * @return the override value, or {@code null} to use the runtime default
         */
        @JsonIgnore
        public OptionalInt getMaxPromptTokens() {
            return maxPromptTokens == null ? OptionalInt.empty() : OptionalInt.of(maxPromptTokens);
        }

        /**
         * Sets the maximum number of tokens in a prompt.
         *
         * @param maxPromptTokens
         *            the override value, or {@code null} to use the runtime default
         * @return this instance for method chaining
         */
        public Limits setMaxPromptTokens(int maxPromptTokens) {
            this.maxPromptTokens = maxPromptTokens;
            return this;
        }
  • Files reviewed: 24/24 changed files
  • Comments generated: 11

Comment on lines 77 to 86
/**
* Returns whether freeform text input is allowed.
*
* @return {@code true} if freeform input is allowed, {@code null} if not
* specified
*/
public Boolean getAllowFreeform() {
return allowFreeform;
@JsonIgnore
public Optional<Boolean> getAllowFreeform() {
return Optional.ofNullable(allowFreeform);
}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in commit that updates @return to describe Optional.empty() when not specified.

Comment on lines 129 to 141
/**
* Gets whether to capture message content as part of telemetry.
* <p>
* Maps to the {@code OTEL_INSTRUMENTATION_GENAI_CAPTURE_MESSAGE_CONTENT}
* environment variable.
*
* @return {@code true} to capture content, {@code false} to suppress it, or
* {@code null} to use the default
*/
public Boolean getCaptureContent() {
return captureContent;
@JsonIgnore
public Optional<Boolean> getCaptureContent() {
return Optional.ofNullable(captureContent);
}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — @return now describes Optional.empty() for the default case.

Comment on lines 298 to 306
/**
* Gets the maximum prompt token override.
*
* @return the max prompt tokens, or {@code null} if not set
*/
public Integer getMaxPromptTokens() {
return maxPromptTokens;
@JsonIgnore
public OptionalInt getMaxPromptTokens() {
return maxPromptTokens == null ? OptionalInt.empty() : OptionalInt.of(maxPromptTokens);
}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — both getMaxPromptTokens() and getMaxOutputTokens() now describe OptionalInt.empty() when unset.

Comment on lines 93 to 131
/**
* Feature flag overrides for model capabilities.
* <p>
* Set a field to {@code true} or {@code false} to override that capability;
* leave it {@code null} to use the runtime default.
*/
@JsonInclude(JsonInclude.Include.NON_NULL)
@JsonIgnoreProperties(ignoreUnknown = true)
public static class Supports {

@JsonProperty("vision")
private Boolean vision;

@JsonProperty("reasoningEffort")
private Boolean reasoningEffort;

/**
* Gets the vision override.
*
* @return {@code true} to enable vision, {@code false} to disable, or
* {@code null} to use the runtime default
*/
public Boolean getVision() {
return vision;
@JsonIgnore
public Optional<Boolean> getVision() {
return Optional.ofNullable(vision);
}

/**
* Sets whether vision (image input) is enabled.
*
* @param vision
* {@code true} to enable, {@code false} to disable, or {@code null}
* to use the runtime default
* @return this instance for method chaining
*/
public Supports setVision(Boolean vision) {
public Supports setVision(boolean vision) {
this.vision = vision;
return this;
}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — updated getter @return tags to describe Optional.empty() and setter @param tags to remove null references. Also added clearXxx() mentions in setter docs.

Comment on lines 671 to 699
/**
* Gets whether automatic configuration discovery is enabled.
*
* @return {@code true} to enable discovery, {@code false} to disable, or
* {@code null} to use the runtime default
*/
public Boolean getEnableConfigDiscovery() {
return enableConfigDiscovery;
@JsonIgnore
public Optional<Boolean> getEnableConfigDiscovery() {
return Optional.ofNullable(enableConfigDiscovery);
}

/**
* Sets whether to automatically discover MCP server configurations and skill
* directories from the working directory.
* <p>
* When {@code true}, the CLI scans the working directory for {@code .mcp.json},
* {@code .vscode/mcp.json} and skill directories, and merges them with
* explicitly provided {@link #setMcpServers(Map)} and
* {@link #setSkillDirectories(List)}, with explicit values taking precedence on
* name collision.
*
* @param enableConfigDiscovery
* {@code true} to enable discovery, {@code false} to disable, or
* {@code null} to use the runtime default
* @return this config instance for method chaining
*/
public SessionConfig setEnableConfigDiscovery(Boolean enableConfigDiscovery) {
public SessionConfig setEnableConfigDiscovery(boolean enableConfigDiscovery) {
this.enableConfigDiscovery = enableConfigDiscovery;
return this;
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — getter @return now describes Optional.empty() and setter @param no longer references null.

Comment on lines 165 to 168
var request = SessionRequestBuilder.buildCreateRequest(config);

assertTrue(Boolean.TRUE.equals(request.getRequestElicitation()));
assertTrue(request.getRequestElicitation());
}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed both assertions (lines 168 and 187) to use assertEquals(Boolean.TRUE, ...) to avoid NPE on null regression.

Comment on lines 502 to 520
/**
* Gets the server-wide idle timeout for sessions in seconds.
*
* @return the session idle timeout in seconds, or {@code null} to disable
* (sessions live indefinitely)
* @return an {@link OptionalInt} containing the session idle timeout in
* seconds, or empty to disable (sessions live indefinitely)
* @since 1.3.0
*/
public Integer getSessionIdleTimeoutSeconds() {
return sessionIdleTimeoutSeconds;
@JsonIgnore
public OptionalInt getSessionIdleTimeoutSeconds() {
return sessionIdleTimeoutSeconds == null ? OptionalInt.empty() : OptionalInt.of(sessionIdleTimeoutSeconds);
}

/**
* Sets the server-wide idle timeout for sessions in seconds.
* <p>
* Sessions without activity for this duration are automatically cleaned up. Set
* to {@code 0} or leave as {@code null} to disable (sessions live
* indefinitely).
* to {@code 0} to disable (sessions live indefinitely). Use
* {@link #clearSessionIdleTimeoutSeconds()} to revert to the default.
* <p>
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aligned the documentation — getter @return now says OptionalInt.empty() if not set and refers to clearSessionIdleTimeoutSeconds() to revert. The setter docs still describe 0 as disabling the timeout (since setSessionIdleTimeoutSeconds(0) stores a present OptionalInt(0) which semantically means "disabled").

Comment on lines 237 to 266
/**
* Enables or disables internal session telemetry for this session. When
* {@code false}, disables session telemetry. When {@code null} (the default) or
* {@code true}, telemetry is enabled for GitHub-authenticated sessions. When a
* custom {@link ProviderConfig} (BYOK) is configured, session telemetry is
* always disabled regardless of this setting. This is independent of
* {@link com.github.copilot.sdk.json.CopilotClientOptions#getTelemetry()
* CopilotClientOptions.TelemetryConfig}, which configures OpenTelemetry export
* for observability.
*
* @return whether session telemetry is enabled
*/
public Boolean getEnableSessionTelemetry() {
return enableSessionTelemetry;
@JsonIgnore
public Optional<Boolean> getEnableSessionTelemetry() {
return Optional.ofNullable(enableSessionTelemetry);
}

/**
* Enables or disables internal session telemetry for this session. When
* {@code false}, disables session telemetry. When {@code null} (the default) or
* {@code true}, telemetry is enabled for GitHub-authenticated sessions. When a
* custom {@link ProviderConfig} (BYOK) is configured, session telemetry is
* always disabled regardless of this setting.
*
* @param enableSessionTelemetry
* whether to enable session telemetry
* @return this config for method chaining
*/
public ResumeSessionConfig setEnableSessionTelemetry(Boolean enableSessionTelemetry) {
public ResumeSessionConfig setEnableSessionTelemetry(boolean enableSessionTelemetry) {
this.enableSessionTelemetry = enableSessionTelemetry;
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — getter Javadoc now describes Optional.empty() for the default, and setter Javadoc replaces null with "unset".

Comment on lines 199 to 207
/**
* Gets whether inference mode is enabled.
*
* @return the infer flag, or {@code null} if not set
*/
public Boolean getInfer() {
return infer;
@JsonIgnore
public Optional<Boolean> getInfer() {
return Optional.ofNullable(infer);
}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — @return now describes Optional.empty() when not set.

Comment on lines 52 to 81
/** Gets the minimum character length. @return the min length */
public Integer getMinLength() {
return minLength;
@JsonIgnore
public OptionalInt getMinLength() {
return minLength == null ? OptionalInt.empty() : OptionalInt.of(minLength);
}

/**
* Sets the minimum character length. @param minLength the min length @return
* this
*/
public InputOptions setMinLength(Integer minLength) {
public InputOptions setMinLength(int minLength) {
this.minLength = minLength;
return this;
}

/**
* Clears the minLength setting, reverting to the default behavior.
*
* @return this instance for method chaining
*/
public InputOptions clearMinLength() {
this.minLength = null;
return this;
}

/** Gets the maximum character length. @return the max length */
public Integer getMaxLength() {
return maxLength;
@JsonIgnore
public OptionalInt getMaxLength() {
return maxLength == null ? OptionalInt.empty() : OptionalInt.of(maxLength);
}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — both getMinLength() and getMaxLength() now describe OptionalInt.empty() when unset.

Copilot AI and others added 2 commits May 13, 2026 21:10
- Update @return tags to describe Optional.empty()/OptionalInt.empty() instead of null
- Update @param tags to remove null references on primitive setters
- Fix exception message in CopilotSession to use valid Java syntax
- Fix ElicitationTest to use assertEquals(Boolean.TRUE, ...) to avoid NPE

Co-authored-by: edburns <75821+edburns@users.noreply.github.com>
Adds tests for getAsInt/get/orElse value retrieval on all Optional-returning
getters, and JSON deserialization round-trip tests for ModelCapabilitiesOverride
inner classes and InfiniteSessionConfig to keep coverage above 83%.

Co-authored-by: edburns <75821+edburns@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use Optional return types instead of nullable boxed primitives in public API

3 participants